Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3: revert ":sparkles: v3 (feature): use any as default Message type of Error struct (#1925)" #2000

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

efectn
Copy link
Member

@efectn efectn commented Aug 7, 2022

It was good idea to make Message type interface{} to improve Fiber error handling. However, it makes much allocations and it's big problem.
If you have better idea to make Fiber error handling better by protecting Fiber performance, please share it 💯

@efectn efectn added this to the v3 milestone Aug 7, 2022
@efectn efectn changed the title Revert ":sparkles: v3 (feature): use any as default Message type of Error struct (#1925)" v3: revert ":sparkles: v3 (feature): use any as default Message type of Error struct (#1925)" Aug 7, 2022
@wangjq4214
Copy link
Member

What do you think about as follows:

type ErrorMine struct {
	Code    int    `json:"code"`
	Message string `json:"message"`
	Data    []any  `json:"data"`
}

func (e *ErrorMine) Error() string {
	if e.Data == nil {
		return e.Message
	}

	return fmt.Sprint(e.Message, " ", e.Data)
}

func NewErrorMine(code int, message string, data ...any) *ErrorMine {
	e := &ErrorMine{
		Code:    code,
		Message: message,
		Data:    data,
	}

	return e
}

In my tests, if no data is passed, the performance is the same as the original design. If data is passed the performance drops severely, even 3.5 times as much as the new design. With the flame chart I can see that the performance consumption is in the fmt.Sprint function.

image

Moreover, maybe zap which is a log libary can provide some useful ideas. Like this API:

logger.Info("failed to fetch URL",
  // Structured context as strongly typed Field values.
  zap.String("url", url),
  zap.Int("attempt", 3),
  zap.Duration("backoff", time.Second),
)

@efectn
Copy link
Member Author

efectn commented Aug 7, 2022

What do you think about as follows:

type ErrorMine struct {
	Code    int    `json:"code"`
	Message string `json:"message"`
	Data    []any  `json:"data"`
}

func (e *ErrorMine) Error() string {
	if e.Data == nil {
		return e.Message
	}

	return fmt.Sprint(e.Message, " ", e.Data)
}

func NewErrorMine(code int, message string, data ...any) *ErrorMine {
	e := &ErrorMine{
		Code:    code,
		Message: message,
		Data:    data,
	}

	return e
}

In my tests, if no data is passed, the performance is the same as the original design. If data is passed the performance drops severely, even 3.5 times as much as the new design. With the flame chart I can see that the performance consumption is in the fmt.Sprint function.

image

Moreover, maybe zap which is a log libary can provide some useful ideas. Like this API:

logger.Info("failed to fetch URL",
  // Structured context as strongly typed Field values.
  zap.String("url", url),
  zap.Int("attempt", 3),
  zap.Duration("backoff", time.Second),
)

I've thought this also. If someone wants to use Data they can customize error handler.
Also we can make a chaining API like: NewErrors().Set("john", "doe") for Data field instead of adding extra parameter to NewErrors.

@trim21
Copy link
Contributor

trim21 commented Aug 7, 2022

I'd like to suggest another thing

#1941

(Although your want a performance improvement suggestion, that pr will affect performance…)

And IMO users can use a more complex error implement and write their own global error handler if they need to use error as HTTP error response (this is currently I'm doing).

A simple built-in fiber Error with code and message is enough, no need to add a data filed.

@ReneWerner87 ReneWerner87 merged commit 4115929 into v3-beta Aug 8, 2022
@efectn efectn deleted the v3-fix-errors branch August 8, 2022 06:50
@ReneWerner87
Copy link
Member

What do you think about as follows:

type ErrorMine struct {
	Code    int    `json:"code"`
	Message string `json:"message"`
	Data    []any  `json:"data"`
}

func (e *ErrorMine) Error() string {
	if e.Data == nil {
		return e.Message
	}

	return fmt.Sprint(e.Message, " ", e.Data)
}

func NewErrorMine(code int, message string, data ...any) *ErrorMine {
	e := &ErrorMine{
		Code:    code,
		Message: message,
		Data:    data,
	}

	return e
}

In my tests, if no data is passed, the performance is the same as the original design. If data is passed the performance drops severely, even 3.5 times as much as the new design. With the flame chart I can see that the performance consumption is in the fmt.Sprint function.

image

Moreover, maybe zap which is a log libary can provide some useful ideas. Like this API:

logger.Info("failed to fetch URL",
  // Structured context as strongly typed Field values.
  zap.String("url", url),
  zap.Int("attempt", 3),
  zap.Duration("backoff", time.Second),
)

errors do not need a DATA node in the standard variant

if you need it later, you can create a custom error, as long as it follows the other interface of the error, it should not be a problem

after that you can just handle it in your custom error handler

@wangjq4214
Copy link
Member

wangjq4214 commented Aug 8, 2022

errors do not need a DATA node in the standard variant

if you need it later, you can create a custom error, as long as it follows the other interface of the error, it should not be a problem

after that you can just handle it in your custom error handler

@ReneWerner87 I agree with you and I do the same as you said in my projects. It's just that the code being revert has this design, so I discussed a possibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants